Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spark-on-k8s sensor - add driver logs #10023

Merged
merged 6 commits into from
Aug 26, 2020

Conversation

bbenzikry
Copy link
Contributor

Simple implementation for appending driver pod logs on completion when using SparkKubernetesSensor.
Originally we discussed monitoring the pod as part of the operator, similar to the implementation for the k8s pod launcher but decided against it for now so we don't introduce breaking changes.

@mik-laj
Copy link
Member

mik-laj commented Jul 27, 2020

@roitvt Can I ask for a review?

@roitvt
Copy link
Contributor

roitvt commented Jul 28, 2020

@roitvt Can I ask for a review?

For sure I'll do it tomorrow :)

@bbenzikry
Copy link
Contributor Author

@roitvt, sorry to bother you - do you think you'll have time to take a look this week?

@roitvt
Copy link
Contributor

roitvt commented Aug 2, 2020

@roitvt, sorry to bother you - do you think you'll have time to take a look this week?

Hi Beni, I'm really sorry I'll do it this week. thank you very much for adding this functionality and I'll be glad to have a talk with you about using this integration and Spark on K8s :)

@bbenzikry
Copy link
Contributor Author

@roitvt, sorry to bother you - do you think you'll have time to take a look this week?

Hi Beni, I'm really sorry I'll do it this week. thank you very much for adding this functionality and I'll be glad to have a talk with you about using this integration and Spark on K8s :)

Great, thanks 👍
I'll be happy to discuss, I'm also interested in some design considerations you had when writing the operator.

@roitvt
Copy link
Contributor

roitvt commented Aug 9, 2020

LGTM works well with spark-pi.
Great addition thanks :)
@bbenzikry @mik-laj

@roitvt
Copy link
Contributor

roitvt commented Aug 9, 2020

@roitvt, sorry to bother you - do you think you'll have time to take a look this week?

Hi Beni, I'm really sorry I'll do it this week. thank you very much for adding this functionality and I'll be glad to have a talk with you about using this integration and Spark on K8s :)

Great, thanks 👍
I'll be happy to discuss, I'm also interested in some design considerations you had when writing the operator.

What is your mail?

@bbenzikry
Copy link
Contributor Author

@roitvt, sorry to bother you - do you think you'll have time to take a look this week?

Hi Beni, I'm really sorry I'll do it this week. thank you very much for adding this functionality and I'll be glad to have a talk with you about using this integration and Spark on K8s :)

Great, thanks 👍
I'll be happy to discuss, I'm also interested in some design considerations you had when writing the operator.

What is your mail?

sent on FB

@bbenzikry
Copy link
Contributor Author

@mik-laj Can this be merged?

watcher = watch.Watch()
return (
watcher,
watcher.stream(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9570
Does this problem occur here? What permissions are needed for this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes a role similar to the one below ( specific namespace role or ClusterRole. In any case the operator is constrained to the job namespace )

- apiGroups: [""]
  resources: ["pods", "pods/log"]
  verbs: ["get", "watch", "list"]

It's important to mention that I opted against using the watcher, but left this here for future use ( I'm using read_namespaced_pod_log, which still requires the mentioned permissions )

Should we add a manifest that deals with it for a system test? similar to RBAC yamls from the operator here

In any case I don't think it should block, as it depends on explicitly adding the flag

@mik-laj
Copy link
Member

mik-laj commented Aug 17, 2020

Is this log available in another application? Maybe a better solution would be to add a link to another console? https://airflow.readthedocs.io/en/latest/howto/write-logs.html#external-links

@bbenzikry
Copy link
Contributor Author

Is this log available in another application? Maybe a better solution would be to add a link to another console? https://airflow.readthedocs.io/en/latest/howto/write-logs.html#external-links

Not necessarily, as it requires either
A) deploying a spark history server in addition to the operator ( we do have that deployed in our cluster, but it's not applicable for any SparkApplication )
B) setting up log forwarding to ES/equivalent from within the spark context
C) forwarding logs for kubernetes pods to an external system

For a user with airflow access alone, we want the ability to quickly inspect driver logs while inspecting the task without delegating to external systems

@mik-laj mik-laj merged commit 1e5aa44 into apache:master Aug 26, 2020
kaxil pushed a commit that referenced this pull request Nov 23, 2020
… call (#11199)

This is a follow up to #10023 - it seems that passing the defined namespace to the log call was missed in one of the rebases done during the PR.
Without this fix, logging will fail when the k8s connection uses a different namespace than the one SparkApplication(s) are actually submitted to.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants